Throw an exception in OOP if an error is logged.#12376
Throw an exception in OOP if an error is logged.#12376davidwengier merged 3 commits intodotnet:mainfrom
Conversation
| _logger.LogWarning($"{SR.FormatEdit_at_deletes(text.GetLinePositionSpan(change.Span), text.ToString(change.Span))}"); | ||
| } | ||
| } | ||
| var message = GetLogMessage(changes, text); |
There was a problem hiding this comment.
Not related to this PR, so no changes requested, but I'm a little surprised to see how this determines if there were problematic changes.
Instead of constructing a new sourcetext and walking it completely, couldn't it instead just look for problems within the changes and the old text?
There was a problem hiding this comment.
Roslyn will happily produce changes like replacing "\tpublic" with " public", so just checking NewText on the change is not sufficient.
Having said that, I think I made a change recently and we might always do our own diffing before getting to this point, which I think won't produce such changes, so it might be possible to change this now.
Will follow up if possible.
There was a problem hiding this comment.
Just to clarify, this commit outlines the change I was thinking. It seems like it would be ok with the kind of change you mentioned, but I could easily be missing something.
There was a problem hiding this comment.
Oh sorry, I completely misunderstood what you were saying. I can certainly come up with a scenario in my head where that approach would fail, but whether that scenario exists in real life is an entirely different (and likely unanswerable) question. If the formatting tests all pass with that change, I have nothing against taking it.
There was a problem hiding this comment.
It looks like roslyn does push non-whitespace characters between edits, so the commit linked above doesn't work. But, the code can still be quite a bit smarter and just do the validation walk over the affected range.
PR: #12384
|
FYI, I looked through all of the LogError calls and found one spot where we weren't filtering out OperationCancelled, so updated that too. |
3 participants
Fixes the first part of #12223
We used to log messages to the output window in VS when formatting was being abandoned due to bad code. With cohosting that got lost, as the log messages are now in the service hub log files only. This resurrects them and makes them into an info bar, by virtue of the exception handling in Roslyn, so users will know what is going on.